-
Notifications
You must be signed in to change notification settings - Fork 41
Conversation
Co-authored-by: Velenir <Velenir@users.noreply.github.com>
Co-authored-by: Velenir <Velenir@users.noreply.github.com>
@@ -175,18 +176,56 @@ export function getTokensFactory(factoryParams: { | |||
const tokenDetailsPromises: (Promise<TokenDetails | undefined> | TokenDetails)[] = [] | |||
addressToIdMap.forEach((id, tokenAddress) => { | |||
// Resolve the details using the config, otherwise fetch the token | |||
const token: TokenDetails | undefined | Promise<TokenDetails | undefined> = tokensConfigMap.has(tokenAddress) | |||
? tokensConfigMap.get(tokenAddress) | |||
const token: undefined | Promise<TokenDetails | undefined> = tokensConfigMap.has(tokenAddress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we in the future call unresolved vars promised<varName>
? I find it easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider #1517
token.then((token) => { | ||
if (token) { | ||
token.label = safeTokenName(token) | ||
} | ||
return token | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we don't await this specific promise, it could be a race condition
It won't be a race condition in our case because this microtask is scheduled before await Promise.all
, but still a bad approach.
if (a.label < b.label) { | ||
return -1 | ||
} else if (a.label > b.label) { | ||
return 1 | ||
} | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering what a lable may be (unicode string)and for simpicity, I recommend String.localeCompare
(or Intl.collator.compare
if you fancy that)
function _moveTokenToHeadOfArray(tokenAddress: string, tokenList: TokenDetails[]): TokenDetails[] { | ||
const tokenIndex = tokenList.findIndex((t) => t.address === tokenAddress) | ||
if (tokenIndex !== -1) { | ||
const token = tokenList[tokenIndex] | ||
tokenList.splice(tokenIndex, 1) | ||
|
||
return [token, ...tokenList] | ||
} | ||
|
||
return tokenList | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, no 😢
This is like 3 loops more than necessary
// Sort tokens | ||
const tokenList = _sortTokens(networkId, tokenDetails) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is misleading, Sort is mutative, so we don't get a new tokenList
out of that. One might be inclined to treat tokenDetails
as unchanged after this line, though we don't do it.
Just writing
_sortTokens(networkId, tokenDetails)
explicitly shows that tokenDetails
was mutated
tokensSorted = _moveTokenToHeadOfArray(WETH_ADDRESS_XDAI, tokensSorted) | ||
return _moveTokenToHeadOfArray(WXDAI_ADDRESS_XDAI, tokensSorted) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, no no 😭
return tokenList | ||
} | ||
|
||
function _sortTokens(networkId: number, tokens: TokenDetails[]): TokenDetails[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do the same in one pass.
Solves the issue that the sorting is not deterministic.
Changing the implementation on how we get the tokens exposed that our order is not deterministic.
When we changed how we iterate and compose the final list of tokens, it moved our token orders.
safeTokenName
NOTE:
label
was added, not only for sorting. I think that it's just nice to have already a not nullable displayable name for debugging and for the UI. We use this function over and over in many parts of the app. This PR doesn't do the refactor to take advantage of this new property. There will be a separate issue for that